-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[uss_qualifier/documentation] Split clean_workspace fragment into fragments for different entity types #850
[uss_qualifier/documentation] Split clean_workspace fragment into fragments for different entity types #850
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, there are just two things I'm not sure were intended for that PR? See comments.
@@ -586,21 +586,28 @@ def _update_links(element: marko.element.Element, origin_filename: str) -> None: | |||
def _add_section_numbers(elements: Sequence[marko.element.Element]) -> None: | |||
heading_level = 2 | |||
levels = [0] | |||
headings = [None] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the changes in that file expected? They are not covered by the PR description.
But they LGTM in themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not mean to include these changes in this PR, but given you've reviewed them now and LGTM, I'll update the description to mention them and then keep them in the PR.
### [Ensure clean workspace test step](clean_workspace.md) | ||
### Ensure clean workspace test step | ||
|
||
<!-- TODO(Shastick): Why do we need to reclean the workspace at this point? We already ensured it was clean before starting the test; don't we know exactly what happened in the test and therefore know that it's already clean (or something failed)? If a previous test case created something that we don't need/want in later test cases, the original test case should clean up at the end of the test case. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the addition of those TODOs intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- it seemed difficult to create separate Issues that referred to exactly the right place and it may certainly be the case that these TODOs are not actually problems. My in-between solution was to add these with the idea that Julien can search for TODO(Shastick) when he returns and address them then.
Previously, the clean_workspace.md test step fragment was a catch-all that documented cleaning out subscriptions, operational intent references, and constraint references regardless of whether all those different entity types were cleaned out or needed to be cleaned out. This PR splits clean_workspace.md into clean_workspace_subs.md, clean_workspace_op_intents.md, and clean_workspace_constraints.md and narrows each test scenario leveraging that fragment to a tighter scope describing what is actually happening.
In a few test scenarios, the code to clear subscriptions is removed because there is no need to have subscriptions removed in order to accomplish the test.
A few hidden TODOs are added to address later. Mislabeled constraint variables and comments in a scenario are fixed.
A small amount of improved error description code for the globally expanded report is included.